Skip to content

Remove outdated and non-generated apply configurations#589

Closed
erikgb wants to merge 1 commit intocert-manager:mainfrom
erikgb:revert-applyconfigurations
Closed

Remove outdated and non-generated apply configurations#589
erikgb wants to merge 1 commit intocert-manager:mainfrom
erikgb:revert-applyconfigurations

Conversation

@erikgb
Copy link
Member

@erikgb erikgb commented Apr 11, 2025

Somehow applyconfiguration-gen got lost in makefile-module improvements. This means that the applyconfigurations are no longer kept up-to-date with the trust-manager APIs. I would personally prefer to fix the generate issue, but I seem to be the only maintainer who likes the ACs. And since they are unused, at least in this project, let's delete them.

Reverts most of #217, but keeps the genclient markers in case users are generating something.

Hopefully, there will be more appetite for ACs when kubernetes-sigs/controller-tools#818 is released.

@erikgb
Remove outdated and non-generated apply configurations
8a281fb 
Signed-off-by: Erik Godding Boye <egboye@gmail.com>
@cert-manager-prow cert-manager-prow bot added the dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. label Apr 11, 2025
@cert-manager-prow
Copy link
Contributor

cert-manager-prow bot commented Apr 11, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jakexks for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cert-manager-prow cert-manager-prow bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 11, 2025
@erikgb erikgb requested a review from inteon April 11, 2025 18:59
@erikgb
Copy link
Member Author

erikgb commented Apr 14, 2025 •
edited
Loading

I just noticed someone is actually using apply configurations in this project, ref. cert-manager/cert-manager#5783 (comment). Maybe we should fix the generator instead? WDYT @inteon?

/hold

@cert-manager-prow cert-manager-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 14, 2025
@inteon
Copy link
Member

inteon commented Apr 14, 2025

I just noticed someone is actually using apply configurations in this project, ref. cert-manager/cert-manager#5783 (comment). Maybe we should fix the generator instead? WDYT @inteon?

/hold

Yes, let's maybe do that, applyconfiguration-gen should be available through makefile-modules.
I think there is a valid usecase for these resources, just by this repo necessarily (since the trust-manager controller only updates the status of the Bundles). As long as we don't have a separate API package, we should keep these up-to-date here.

@erikgb erikgb mentioned this pull request Apr 14, 2025
Merged
@erikgb
Copy link
Member Author

erikgb commented Apr 14, 2025

I just noticed someone is actually using apply configurations in this project, ref. cert-manager/cert-manager#5783 (comment). Maybe we should fix the generator instead? WDYT @inteon?
/hold

Yes, let's maybe do that, applyconfiguration-gen should be available through makefile-modules. I think there is a valid usecase for these resources, just by this repo necessarily (since the trust-manager controller only updates the status of the Bundles). As long as we don't have a separate API package, we should keep these up-to-date here.

Agreed! Here is an alternative PR: #598. PTAL!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@inteon inteon Awaiting requested review from inteon

Assignees

No one assigned

Labels

dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@erikgb @inteon